go-metrics: pull library and introduce ResettingTimer and InfluxDB reporter. #15910

nonsense · 2018-01-17T13:01:28Z

This PR introduces the following changes:

updates the rcrowley/go-metrics to its latest version
fork github.com/rcrowley/go-metrics to github.com/ethersphere/go-metrics, in order to:
add a ResettingTimer metric type which calculates mean, 50%ile, 95%ile, 99%ile, min, and max for timers per flush interval - timers are reset on every Snapshot() call.
vendor github.com/ethersphere/go-metrics-influxdb, which includes an InfluxDB reporter - to be used to flush metrics registry to an InfluxDB backend every N seconds.

These changes are necessary because we want to be able to measure latencies in swarm (and probably at some point in geth?) and forward them to a time-series backend (InfluxDB) and visualise them later with Grafana. The existing Timer type is not suitable for this, see http://taint.org/2014/01/16/145944a.html or https://groups.google.com/d/msg/mechanical-sympathy/I4JfZQ1GYi8/Bf1ftrjpuKMJ .

TODO:

add tests for ResettingTimer
use the up-to-date InfluxDB client at https://github.com/influxdata/influxdb/tree/master/client/v2 instead of https://github.com/influxdata/influxdb/tree/master/client which is deprecated.
move logic which calculates mean, 50%ile, 95%ile, 99%ile, min, and max away from the reporter into the ResettingTimer.

lmars · 2018-01-17T13:07:15Z

consensus/ethash/ethash.go

@@ -35,7 +35,7 @@ import (
 	"github.com/ethereum/go-ethereum/consensus"
 	"github.com/ethereum/go-ethereum/log"
 	"github.com/ethereum/go-ethereum/rpc"
-	metrics "github.com/rcrowley/go-metrics"
+	metrics "github.com/nonsense/go-metrics"


Should we fork to ethereum or ethersphere instead so the team can manage this dependency? I've just hit this issue in the past where a user's fork needs to be forked again to make changes.

Good point. I'm happy with any repo. ethersphere or ethereum would both work. If we don't want to keep go-metrics and go-metrics-influxdb on ethereum, I would be happy to fork them on ethersphere so that at least there are more people who have access to it.

Actually my first thought was to vendor it like the log package which you pointed out, but there is already a metrics package which does other things, and having the go-metrics under the metrics folder seemed weird. But maybe we can vendor all the metrics related libraries under metrics/lib in go-ethereum?

Why do we need to fork go-metrics? Can't we just add one more metric of our own (ResettingTimer)?

We would also have to create our own Registry (see https://github.com/ethereum/go-ethereum/pull/15910/files#diff-69f4fcff395026c7300bbaa7c953e251R199), when the StandardRegistry is good for our purposes.

In my opinion metrics library is something that doesn't change often, so I'd rather keep all related code in one library, rather than patch additional functionality with external structs. Because of this I found forking to be the easiest. I'm happy to hear other opinions on this though.

@lmars changed fork location to ethersphere.

nonsense · 2018-01-23T10:46:04Z

vendor/github.com/ethersphere/go-metrics/resetting_timer.go

@@ -0,0 +1,237 @@
+package metrics


This is the main change in this PR. Since govendor does not include tests, you can find them at https://github.com/ethersphere/go-metrics/blob/master/resetting_timer_test.go

…porter.

nonsense · 2018-02-07T17:04:39Z

@lmars @karalabe I rebased this on master so that it is up-to-date. AppVeyor seems to be crashing with unrelated code:

[00:21:22] fatal error: out of memory
[00:21:22] FAIL	github.com/ethereum/go-ethereum/tests	209.548s

Is there anything else you expect from this PR? Are you happy with it in general, or shall I fix something before we merge? I believe we can use the current InfluxDB client for the time being as it works fine.

lmars · 2018-02-08T02:19:58Z

LGTM

nonsense · 2018-02-22T11:12:09Z

@karalabe as discussed I pulled the go-metrics repo in go-ethereum, rather than vendoring it.

The changes introduced are as follows:

go-metrics had UseNilMetrics global bool, which was identical to go-ethereum's Enabled, so I got rid of UseNilMetrics and used Enabled instead.
go-ethereum code used a custom constructor for measurements, which was in contradiction with the go-metrics API, so I changed all NewCounter, etc. to NewRegisteredCounter passing the default registry.
Introduced package exp in order to solve a circular dependency and init the metrics/exp package. This was necessary as I merged our go-ethereum metrics package with the go-metrics library.
Pulled in the influxdb reporter, as it is just one file, and doesn't make sense to maintain a fork for it either.
Vendored the influxdb client, which is supported by InfluxData, and used by the influxdb reporter mentioned in 4.

cc @lmars

karalabe · 2018-02-22T16:43:06Z

exp/exp.go

+
+func init() {
+	e.Exp(metrics.DefaultRegistry)
+}


I don't think you wanted to commit this file :)

Or I don't get it why this is outside in its own package vs underneath metrics?

Currently we have https://github.com/ethereum/go-ethereum/blob/master/metrics/metrics.go#L48 . Since we are merging go-ethereum metrics and go-metrics in one package, we can't have this initialisation in the new joint metrics as it results in circular dependency:

metrics -> exp -> metrics

Then again I don't think we want a separate package just for this, so it can go anywhere else but in the metrics package.

Well, since it's our custom package now, we can just do this call within metrics/exp/exp.go as an init. Would that be bad?

Sure, I will move it to metrics/exp/exp.go. What I am not very happy about is that we are starting an HTTP server in this file, which is kind of hidden away, and we would always be initialising the /debug/metrics endpoint with the default registry.

I can imagine that at some point we might want to have more than one registry (default registry, swarm registry, etc.), but I guess for now every binary can just use the default registry for simplicity.

~~So for now metrics/exp/exp.go should be fine to init this server, but we might want to refactor in the future.~~

This endpoint is exposed as part of the pprof http server (see 9e9bfc4), so I decided to set it up next to the pprof http server.

Moving it to metrics/exp/exp.go is not a solution, because no package is including exp so the init would never be called. We need another package to import the exp package, so internal/debug seems to be the correct place.

@karalabe I hope that makes sense.

… server

karalabe

LGTM

…ter (ethereum#15910) * go-metrics: fork library and introduce ResettingTimer and InfluxDB reporter. * vendor: change nonsense/go-metrics to ethersphere/go-metrics * go-metrics: add tests. move ResettingTimer logic from reporter to type. * all, metrics: pull in metrics package in go-ethereum * metrics/test: make sure metrics are enabled for tests * metrics: apply gosimple rules * metrics/exp, internal/debug: init expvar endpoint when starting pprof server * internal/debug: tiny comment formatting fix

This change includes a lot of things, listed below. ### Split up interfaces, write vs read The interfaces have been split up into one write-interface and one read-interface, with `Snapshot` being the gateway from write to read. This simplifies the semantics _a lot_. Example of splitting up an interface into one readonly 'snapshot' part, and one updatable writeonly part: ```golang type MeterSnapshot interface { Count() int64 Rate1() float64 Rate5() float64 Rate15() float64 RateMean() float64 } // Meters count events to produce exponentially-weighted moving average rates // at one-, five-, and fifteen-minutes and a mean rate. type Meter interface { Mark(int64) Snapshot() MeterSnapshot Stop() } ``` ### A note about concurrency This PR makes the concurrency model clearer. We have actual meters and snapshot of meters. The `meter` is the thing which can be accessed from the registry, and updates can be made to it. - For all `meters`, (`Gauge`, `Timer` etc), it is assumed that they are accessed by different threads, making updates. Therefore, all `meters` update-methods (`Inc`, `Add`, `Update`, `Clear` etc) need to be concurrency-safe. - All `meters` have a `Snapshot()` method. This method is _usually_ called from one thread, a backend-exporter. But it's fully possible to have several exporters simultaneously: therefore this method should also be concurrency-safe. TLDR: `meter`s are accessible via registry, all their methods must be concurrency-safe. For all `Snapshot`s, it is assumed that an individual exporter-thread has obtained a `meter` from the registry, and called the `Snapshot` method to obtain a readonly snapshot. This snapshot is _not_ guaranteed to be concurrency-safe. There's no need for a snapshot to be concurrency-safe, since exporters should not share snapshots. Note, though: that by happenstance a lot of the snapshots _are_ concurrency-safe, being unmutable minimal representations of a value. Only the more complex ones are _not_ threadsafe, those that lazily calculate things like `Variance()`, `Mean()`. Example of how a background exporter typically works, obtaining the snapshot and sequentially accessing the non-threadsafe methods in it: ```golang ms := metric.Snapshot() ... fields := map[string]interface{}{ "count": ms.Count(), "max": ms.Max(), "mean": ms.Mean(), "min": ms.Min(), "stddev": ms.StdDev(), "variance": ms.Variance(), ``` TLDR: `snapshots` are not guaranteed to be concurrency-safe (but often are). ### Sample changes I also changed the `Sample` type: previously, it iterated the samples fully every time `Mean()`,`Sum()`, `Min()` or `Max()` was invoked. Since we now have readonly base data, we can just iterate it once, in the constructor, and set all four values at once. The same thing has been done for runtimehistogram. ### ResettingTimer API Back when ResettingTImer was implemented, as part of #15910, Anton implemented a `Percentiles` on the new type. However, the method did not conform to the other existing types which also had a `Percentiles`. 1. The existing ones, on input, took `0.5` to mean `50%`. Anton used `50` to mean `50%`. 2. The existing ones returned `float64` outputs, thus interpolating between values. A value-set of `0, 10`, at `50%` would return `5`, whereas Anton's would return either `0` or `10`. This PR removes the 'new' version, and uses only the 'legacy' percentiles, also for the ResettingTimer type. The resetting timer snapshot was also defined so that it would expose the internal values. This has been removed, and getters for `Max, Min, Mean` have been added instead. ### Unexport types A lot of types were exported, but do not need to be. This PR unexports quite a lot of them.

This change includes a lot of things, listed below. ### Split up interfaces, write vs read The interfaces have been split up into one write-interface and one read-interface, with `Snapshot` being the gateway from write to read. This simplifies the semantics _a lot_. Example of splitting up an interface into one readonly 'snapshot' part, and one updatable writeonly part: ```golang type MeterSnapshot interface { Count() int64 Rate1() float64 Rate5() float64 Rate15() float64 RateMean() float64 } // Meters count events to produce exponentially-weighted moving average rates // at one-, five-, and fifteen-minutes and a mean rate. type Meter interface { Mark(int64) Snapshot() MeterSnapshot Stop() } ``` ### A note about concurrency This PR makes the concurrency model clearer. We have actual meters and snapshot of meters. The `meter` is the thing which can be accessed from the registry, and updates can be made to it. - For all `meters`, (`Gauge`, `Timer` etc), it is assumed that they are accessed by different threads, making updates. Therefore, all `meters` update-methods (`Inc`, `Add`, `Update`, `Clear` etc) need to be concurrency-safe. - All `meters` have a `Snapshot()` method. This method is _usually_ called from one thread, a backend-exporter. But it's fully possible to have several exporters simultaneously: therefore this method should also be concurrency-safe. TLDR: `meter`s are accessible via registry, all their methods must be concurrency-safe. For all `Snapshot`s, it is assumed that an individual exporter-thread has obtained a `meter` from the registry, and called the `Snapshot` method to obtain a readonly snapshot. This snapshot is _not_ guaranteed to be concurrency-safe. There's no need for a snapshot to be concurrency-safe, since exporters should not share snapshots. Note, though: that by happenstance a lot of the snapshots _are_ concurrency-safe, being unmutable minimal representations of a value. Only the more complex ones are _not_ threadsafe, those that lazily calculate things like `Variance()`, `Mean()`. Example of how a background exporter typically works, obtaining the snapshot and sequentially accessing the non-threadsafe methods in it: ```golang ms := metric.Snapshot() ... fields := map[string]interface{}{ "count": ms.Count(), "max": ms.Max(), "mean": ms.Mean(), "min": ms.Min(), "stddev": ms.StdDev(), "variance": ms.Variance(), ``` TLDR: `snapshots` are not guaranteed to be concurrency-safe (but often are). ### Sample changes I also changed the `Sample` type: previously, it iterated the samples fully every time `Mean()`,`Sum()`, `Min()` or `Max()` was invoked. Since we now have readonly base data, we can just iterate it once, in the constructor, and set all four values at once. The same thing has been done for runtimehistogram. ### ResettingTimer API Back when ResettingTImer was implemented, as part of ethereum#15910, Anton implemented a `Percentiles` on the new type. However, the method did not conform to the other existing types which also had a `Percentiles`. 1. The existing ones, on input, took `0.5` to mean `50%`. Anton used `50` to mean `50%`. 2. The existing ones returned `float64` outputs, thus interpolating between values. A value-set of `0, 10`, at `50%` would return `5`, whereas Anton's would return either `0` or `10`. This PR removes the 'new' version, and uses only the 'legacy' percentiles, also for the ResettingTimer type. The resetting timer snapshot was also defined so that it would expose the internal values. This has been removed, and getters for `Max, Min, Mean` have been added instead. ### Unexport types A lot of types were exported, but do not need to be. This PR unexports quite a lot of them.

This change includes a lot of things, listed below. ### Split up interfaces, write vs read The interfaces have been split up into one write-interface and one read-interface, with `Snapshot` being the gateway from write to read. This simplifies the semantics _a lot_. Example of splitting up an interface into one readonly 'snapshot' part, and one updatable writeonly part: ```golang type MeterSnapshot interface { Count() int64 Rate1() float64 Rate5() float64 Rate15() float64 RateMean() float64 } // Meters count events to produce exponentially-weighted moving average rates // at one-, five-, and fifteen-minutes and a mean rate. type Meter interface { Mark(int64) Snapshot() MeterSnapshot Stop() } ``` ### A note about concurrency This PR makes the concurrency model clearer. We have actual meters and snapshot of meters. The `meter` is the thing which can be accessed from the registry, and updates can be made to it. - For all `meters`, (`Gauge`, `Timer` etc), it is assumed that they are accessed by different threads, making updates. Therefore, all `meters` update-methods (`Inc`, `Add`, `Update`, `Clear` etc) need to be concurrency-safe. - All `meters` have a `Snapshot()` method. This method is _usually_ called from one thread, a backend-exporter. But it's fully possible to have several exporters simultaneously: therefore this method should also be concurrency-safe. TLDR: `meter`s are accessible via registry, all their methods must be concurrency-safe. For all `Snapshot`s, it is assumed that an individual exporter-thread has obtained a `meter` from the registry, and called the `Snapshot` method to obtain a readonly snapshot. This snapshot is _not_ guaranteed to be concurrency-safe. There's no need for a snapshot to be concurrency-safe, since exporters should not share snapshots. Note, though: that by happenstance a lot of the snapshots _are_ concurrency-safe, being unmutable minimal representations of a value. Only the more complex ones are _not_ threadsafe, those that lazily calculate things like `Variance()`, `Mean()`. Example of how a background exporter typically works, obtaining the snapshot and sequentially accessing the non-threadsafe methods in it: ```golang ms := metric.Snapshot() ... fields := map[string]interface{}{ "count": ms.Count(), "max": ms.Max(), "mean": ms.Mean(), "min": ms.Min(), "stddev": ms.StdDev(), "variance": ms.Variance(), ``` TLDR: `snapshots` are not guaranteed to be concurrency-safe (but often are). ### Sample changes I also changed the `Sample` type: previously, it iterated the samples fully every time `Mean()`,`Sum()`, `Min()` or `Max()` was invoked. Since we now have readonly base data, we can just iterate it once, in the constructor, and set all four values at once. The same thing has been done for runtimehistogram. ### ResettingTimer API Back when ResettingTImer was implemented, as part of ethereum/go-ethereum#15910, Anton implemented a `Percentiles` on the new type. However, the method did not conform to the other existing types which also had a `Percentiles`. 1. The existing ones, on input, took `0.5` to mean `50%`. Anton used `50` to mean `50%`. 2. The existing ones returned `float64` outputs, thus interpolating between values. A value-set of `0, 10`, at `50%` would return `5`, whereas Anton's would return either `0` or `10`. This PR removes the 'new' version, and uses only the 'legacy' percentiles, also for the ResettingTimer type. The resetting timer snapshot was also defined so that it would expose the internal values. This has been removed, and getters for `Max, Min, Mean` have been added instead. ### Unexport types A lot of types were exported, but do not need to be. This PR unexports quite a lot of them.

nonsense requested a review from karalabe as a code owner January 17, 2018 13:01

lmars reviewed Jan 17, 2018

View reviewed changes

nonsense force-pushed the fork-go-metrics-for-resetting-timer branch from 8ac9c5a to 95bd51a Compare January 23, 2018 10:18

nonsense commented Jan 23, 2018

View reviewed changes

nonsense force-pushed the fork-go-metrics-for-resetting-timer branch from 95bd51a to 59de458 Compare January 23, 2018 14:07

nonsense mentioned this pull request Jan 25, 2018

swarm: initial instrumentation #15969

Merged

nonsense added 2 commits February 7, 2018 15:17

go-metrics: fork library and introduce ResettingTimer and InfluxDB re…

9c3be56

…porter.

vendor: change nonsense/go-metrics to ethersphere/go-metrics

e5df069

nonsense force-pushed the fork-go-metrics-for-resetting-timer branch from cf11351 to 0c3d105 Compare February 7, 2018 14:19

nonsense force-pushed the fork-go-metrics-for-resetting-timer branch from 0c3d105 to b2cb5c0 Compare February 7, 2018 17:06

go-metrics: add tests. move ResettingTimer logic from reporter to type.

19e1ba5

nonsense force-pushed the fork-go-metrics-for-resetting-timer branch from b2cb5c0 to 19e1ba5 Compare February 20, 2018 13:40

vendor: resolved merge conflicts

c296da8

nonsense requested review from fjl, holiman and zsfelfoldi as code owners February 22, 2018 11:06

nonsense changed the title ~~go-metrics: fork library and introduce ResettingTimer and InfluxDB reporter.~~ go-metrics: pull library and introduce ResettingTimer and InfluxDB reporter. Feb 22, 2018

nonsense force-pushed the fork-go-metrics-for-resetting-timer branch from 784c2af to bb2e5c2 Compare February 22, 2018 11:46

nonsense added 2 commits February 22, 2018 13:45

all, metrics: pull in metrics package in go-ethereum

c12cfd1

metrics/test: make sure metrics are enabled for tests

c14cd85

nonsense force-pushed the fork-go-metrics-for-resetting-timer branch from bb2e5c2 to c14cd85 Compare February 22, 2018 12:53

metrics: apply gosimple rules

aaef540

karalabe reviewed Feb 22, 2018

View reviewed changes

metrics/exp, internal/debug: init expvar endpoint when starting pprof…

a284474

… server

nonsense force-pushed the fork-go-metrics-for-resetting-timer branch from 3cc72e4 to a284474 Compare February 22, 2018 19:58

internal/debug: tiny comment formatting fix

ff772cc

karalabe approved these changes Feb 23, 2018

View reviewed changes

karalabe added this to the 1.8.2 milestone Feb 23, 2018

karalabe merged commit ae9f972 into ethereum:master Feb 23, 2018

fjl mentioned this pull request Mar 14, 2023

metrics: add cpu counters #26796

Merged

This was referenced Aug 30, 2023

metrics: apply selected changes from upstream #28031

Closed

metrics: refactor metrics (part II) #28035

Merged

trinhdn2 mentioned this pull request Jan 15, 2024

Refactor metrics package with Prometheus and InfluxDB support BuildOnViction/victionchain#428

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go-metrics: pull library and introduce ResettingTimer and InfluxDB reporter. #15910

go-metrics: pull library and introduce ResettingTimer and InfluxDB reporter. #15910

nonsense commented Jan 17, 2018 •

edited

Loading

lmars Jan 17, 2018

nonsense Jan 17, 2018

nonsense Jan 17, 2018

karalabe Jan 17, 2018

nonsense Jan 17, 2018

nonsense Jan 17, 2018

nonsense Jan 19, 2018

nonsense Jan 23, 2018

nonsense commented Feb 7, 2018 •

edited

Loading

lmars commented Feb 8, 2018

nonsense commented Feb 22, 2018 •

edited

Loading

karalabe Feb 22, 2018

karalabe Feb 22, 2018

nonsense Feb 22, 2018 •

edited

Loading

nonsense Feb 22, 2018

karalabe Feb 22, 2018

nonsense Feb 22, 2018 •

edited

Loading

nonsense Feb 22, 2018

karalabe left a comment

go-metrics: pull library and introduce ResettingTimer and InfluxDB reporter. #15910

go-metrics: pull library and introduce ResettingTimer and InfluxDB reporter. #15910

Conversation

nonsense commented Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nonsense commented Feb 7, 2018 • edited Loading

lmars commented Feb 8, 2018

nonsense commented Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nonsense Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nonsense Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karalabe left a comment

Choose a reason for hiding this comment

nonsense commented Jan 17, 2018 •

edited

Loading

nonsense commented Feb 7, 2018 •

edited

Loading

nonsense commented Feb 22, 2018 •

edited

Loading

nonsense Feb 22, 2018 •

edited

Loading

nonsense Feb 22, 2018 •

edited

Loading